doc: add How to write a Node.js test guide#6984
Conversation
doc/guides/writing_tests.md
Outdated
There was a problem hiding this comment.
nit: I’d prefer s/They/Tests/ because there’s no plural tests before this in the text.
|
There’s a “test” missing in the commit message/PR title. Generally looking good, nice work! |
|
Oh, and I think linking this from the corresponding |
Fishrock123
changed the title
How to write a Node.js guideHow to write a Node.js test guide
doc/guides/writing_tests.md
Outdated
There was a problem hiding this comment.
Would "How to write a test for the Node.js project" be better because I thought this is for how to write a test case in Node.js community, not the Node.js project in my first sight on this title. :-)
santigimeno
force-pushed
the
test_guid
branch
3 times, most recently
from
f5bd5fe to
8259de3
Compare
|
LGTM and I still think linking this from the |
|
Oh! I forgot. Adding it... |
santigimeno
force-pushed
the
test_guid
branch
6 times, most recently
from
6b5c7a0 to
9fec0a5
Compare
|
Link added to |
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
There are only very few cases of explicit license notifications in the test code base (and in general) left, so I’d drop the mention here.
There was a problem hiding this comment.
Maybe dropping the whole parenthesis? The common includes info is already in the guide.
There was a problem hiding this comment.
@santigimeno Oh, sorry, didn’t see this was taken directly from the existing text. But yeah, +1 to dropping the whole parenthesis here.
|
LGTM with a comment |
|
Updated. Thanks! |
doc/guides/writing_tests.md
Outdated
|
LGTM. There are definitely some more improvements that can happen in the CONTRIBUTING doc but it's probably best for that to happen in another PR. |
|
Added the missing |
The sentence about the location of the tests could be changed and then explained in some depth in the guide.
Agreed |
PR-URL: nodejs#6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
|
CI: https://ci.nodejs.org/job/node-test-pull-request/2862/. All green except unrelated failures in some ARM bots. Landing |
|
Landed in b83b363. Thanks! |
PR-URL: nodejs#6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
PR-URL: #6984 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
6 participants
Checklist
Affected core subsystem(s)
doc, test
Description of change
Refs: nodejs/testing#30
/cc @nodejs/testing @nodejs/documentation